Skip to content

fix!: normalise InvalidArgumentError to server_error on the wire#451

Open
dhensby wants to merge 1 commit into
node-oauth:masterfrom
dhensby:fix/invalid-argument-error-leak
Open

fix!: normalise InvalidArgumentError to server_error on the wire#451
dhensby wants to merge 1 commit into
node-oauth:masterfrom
dhensby:fix/invalid-argument-error-leak

Conversation

@dhensby

@dhensby dhensby commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

What

InvalidArgumentError carries the non-standard invalid_argument code but extends OAuthError, so the token and authorize handler catch blocks — which only wrap non-OAuthError errors as ServerError — let it pass straight through and serialize it to the client. This normalises InvalidArgumentError to ServerError at the serialisation boundary, so genuine runtime failures surface as a standard server_error instead of leaking invalid_argument.

Resolves #67.

Why

invalid_argument is defined in none of RFC 6749's error sets (§5.2 token endpoint; §4.1.2.1 / §4.2.2.1 authorization endpoint) nor the IANA registry. Of the ~56 InvalidArgumentError throw sites, ~32 are construction/config-time guards thrown straight back to the integrator that never reach a client — for those, invalid_argument is a useful developer signal and stays as-is. The defect is the small subset thrown during request handling, inside the handler try, which currently serialize as error=invalid_argument / HTTP 500:

  • a model returning malformed token data → TokenModel / BearerTokenType throw
  • a model returning a falsy authorization code → CodeResponseType throws

Change

Both handler catch blocks now also coerce InvalidArgumentError to ServerError (preserving the original as .inner):

if (!(e instanceof OAuthError) || e instanceof InvalidArgumentError) {
  e = new ServerError(e);
}

Setup-time guards (missing options, model does not implement X(), request/response instance checks) are thrown before the try block, so they're unaffected and still surface InvalidArgumentError to integrator code.

Tests

Added two integration tests proving the wire output for the reachable paths is now server_error/503 (token endpoint) and an error=server_error redirect (authorize endpoint), with the original InvalidArgumentError retained as .inner. Full suite green; no existing InvalidArgumentError assertion changes (all are on pre-try construction / request-instance paths).

BREAKING CHANGE

When a model returns malformed token data or a falsy authorization code at request time, the OAuth error response now reports error: server_error (HTTP 503) instead of error: invalid_argument (HTTP 500). Setup-time InvalidArgumentErrors thrown to integrator code are unchanged.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a standards-compliance issue where InvalidArgumentError (with non-standard invalid_argument) could leak to OAuth clients during request handling, by coercing it to a ServerError at the handler serialization boundary while preserving the original error as .inner.

Changes:

  • Normalize InvalidArgumentError to ServerError in TokenHandler and AuthorizeHandler catch blocks (request-handling path only).
  • Add integration tests asserting server_error is returned on the wire for the reachable runtime InvalidArgumentError cases (token response body + status, authorize redirect query params).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
lib/handlers/token-handler.js Wraps request-time InvalidArgumentError as ServerError before updating the token error response.
lib/handlers/authorize-handler.js Wraps request-time InvalidArgumentError as ServerError before building the authorization error redirect URI.
test/integration/handlers/token-handler_test.js Adds integration coverage ensuring token endpoint returns server_error/503 and retains the original error as .inner.
test/integration/handlers/authorize-handler_test.js Adds integration coverage ensuring authorize endpoint redirects with error=server_error and retains the original error as .inner.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@dhensby dhensby requested a review from jankapunkt June 24, 2026 09:52
@jankapunkt

Copy link
Copy Markdown
Member

While this is non-breaking in terms of the OAuth workflow, it might break existing systems that attempt to catch InvalidArgumentError - although I think this is not very likely. How do you feel about this @dhensby ?

@dhensby

dhensby commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

I agree, but feel like it should be a major release out of caution for those users.

@dhensby dhensby force-pushed the fix/invalid-argument-error-leak branch from 83abd6a to fac13b8 Compare June 24, 2026 11:20
@jankapunkt

Copy link
Copy Markdown
Member

While I agree on that as well (and I think with #399 we are anyway forced to bump major) I wonder about the maintenance of older major versions, especially 4.x for which we were already merging back some newer fixes. While I personally value this a lot, this can get easily out of hand if we have too many major version branches to maintain.

@dhensby

dhensby commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

I'm also going to open a PR that supersedes #336 and that will be breaking too, so I think we should think about a new major soon.

If we implement the semantic-release PR #335 - then support for older versions falls out of that by creating 4.x 5.x, etc release branches and we can be intentional about our back-ported support.

InvalidArgumentError carries the non-standard `invalid_argument` code (defined in
neither RFC 6749 §5.2 nor §4.1.2.1) but extends OAuthError, so the token and
authorize handler catch blocks let it slip through their "wrap any non-OAuthError
as ServerError" guard. As a result, a misbehaving model that returns malformed
token data (e.g. a non-Date `accessTokenExpiresAt`) or a falsy authorization code
caused the server to emit `error=invalid_argument` with HTTP 500 to the client
instead of a standard `server_error`.

Treat InvalidArgumentError like any other internal error at the serialisation
boundary: normalise it to ServerError before it reaches the client, preserving
the original as `.inner`. Construction- and request-validation guards (missing
options, model-capability checks, request/response instance checks) are thrown
before the try block and are unaffected — they still surface InvalidArgumentError
to the integrator as a descriptive developer-facing error.

BREAKING CHANGE: when a model returns malformed token data or a falsy
authorization code at request time, the OAuth error response now reports
`error: server_error` with HTTP status 503 instead of `error: invalid_argument`
with HTTP status 500. Setup-time InvalidArgumentErrors thrown to integrator code
are unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dhensby dhensby force-pushed the fix/invalid-argument-error-leak branch from 256e2dd to 1724f63 Compare June 24, 2026 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace InvalidArgumentError with ServerError

3 participants